[SPARK-17480][SQL][FOLLOWUP] Fix more instances which calls List.length/size which is O(n) by HyukjinKwon · Pull Request #15093 · apache/spark

HyukjinKwon · 2016-09-14T13:06:39Z

What changes were proposed in this pull request?

This PR fixes all the instances which was fixed in the previous PR.

To make sure, I manually debugged and also checked the Scala source. length in LinearSeqOptimized.scala#L49-L57 is O(n). Also, size calls length via SeqLike.scala#L106.

For debugging, I have created these as below:

ArrayBuffer(1, 2, 3)
Array(1, 2, 3)
List(1, 2, 3)
Seq(1, 2, 3)

and then called size and length for each to debug.

How was this patch tested?

I ran the bash as below on Mac

find . -name *.scala -type f -exec grep -il "while (.*\\.length)" {} \; | grep "src/main"
find . -name *.scala -type f -exec grep -il "while (.*\\.size)" {} \; | grep "src/main"

and then checked each.

HyukjinKwon · 2016-09-14T13:08:09Z

@srowen Could you please review this?

srowen · 2016-09-14T13:21:16Z

mllib/src/main/scala/org/apache/spark/mllib/linalg/distributed/RowMatrix.scala

      gamma: Double): CoordinateMatrix = {
    require(gamma > 1.0, s"Oversampling should be greater than 1: $gamma")
-    require(colMags.size == this.numCols(), "Number of magnitudes didn't match column dimension")
+    require(colMags.length == this.numCols(), "Number of magnitudes didn't match column dimension")


This is different, yeah. You're just avoiding the implicit conversion. While that's sort of good I think it's separate and maybe something to change or not change everywhere at once.

Sure! I will revert this change.

HyukjinKwon · 2016-09-14T13:39:35Z

@srowen Please let me check this out again tomorrow.

SparkQA · 2016-09-14T15:12:27Z

Test build #65366 has finished for PR 15093 at commit ecd3dde.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-09-14T15:53:42Z

Test build #65369 has finished for PR 15093 at commit 996e292.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-09-15T12:16:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

-      var i = 0
-      while (i < groupedWindowExpressions.size) {
-        val ((partitionSpec, orderSpec), windowExpressions) = groupedWindowExpressions(i)
+      groupedWindowExpressions.foreach { case ((partitionSpec, orderSpec), windowExpressions) =>


Hi @srowen , It seems groupedWindowExpressions is Seq[((Seq[Expression], Seq[SortOrder]), ArrayBuffer[NamedExpression])] but the desired output is LogicalPlan. So, it seems I can't directly use fold or reduce simply. So, could this just use foreach rather than others? Otherwise, please teach me for a better expression.

Well currentChild is a Window, and that's the only thing whose computation is changed here. How about:

val currentChild = groupedWindowExpressions.foldLeft(child) { case (last, ((partitionSpec, orderSpec), windowExpressions)) => Window(windowExpressions, partitionSpec, orderSpec, last) }

Ah, that's clear and better. I will try. Thanks!

SparkQA · 2016-09-15T15:14:19Z

Test build #65440 has finished for PR 15093 at commit cb536a4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-09-16T11:09:45Z

OK, looks good.

Some of these are .size -> .length changes, but I suppose those are related and positive. It avoids an extra method invocation, which might matter in a very tight loop.

srowen · 2016-09-17T11:46:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+      val currentChild =
+        groupedWindowExpressions.foldLeft(child) {
+          case (last, ((partitionSpec, orderSpec), windowExpressions)) =>
+            // Set currentChild to the newly created Window operator.


Sorry, one last thing, can you remove this comment? and update the comment above and below this expression? they're no longer applicable. Actually currentChild isn't a great name now anyway. eh, windowOps or something?

Ah, yes, I should've checked this closely. I will update soon.

SparkQA · 2016-09-17T15:05:46Z

Test build #65535 has finished for PR 15093 at commit 8a3d293.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-09-17T15:11:12Z

retest this please

SparkQA · 2016-09-17T15:48:09Z

Test build #65534 has finished for PR 15093 at commit f5e8131.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-09-17T15:54:03Z

Merging to master, and to 2.0 to match the parent JIRA, even though this is probably less important, yet still a clean small win

…th/size which is O(n) This PR fixes all the instances which was fixed in the previous PR. To make sure, I manually debugged and also checked the Scala source. `length` in [LinearSeqOptimized.scala#L49-L57](https://github.com/scala/scala/blob/2.11.x/src/library/scala/collection/LinearSeqOptimized.scala#L49-L57) is O(n). Also, `size` calls `length` via [SeqLike.scala#L106](https://github.com/scala/scala/blob/2.11.x/src/library/scala/collection/SeqLike.scala#L106). For debugging, I have created these as below: ```scala ArrayBuffer(1, 2, 3) Array(1, 2, 3) List(1, 2, 3) Seq(1, 2, 3) ``` and then called `size` and `length` for each to debug. I ran the bash as below on Mac ```bash find . -name *.scala -type f -exec grep -il "while (.*\\.length)" {} \; | grep "src/main" find . -name *.scala -type f -exec grep -il "while (.*\\.size)" {} \; | grep "src/main" ``` and then checked each. Author: hyukjinkwon <gurwls223@gmail.com> Closes #15093 from HyukjinKwon/SPARK-17480-followup. (cherry picked from commit 86c2d39) Signed-off-by: Sean Owen <sowen@cloudera.com>

SparkQA · 2016-09-17T17:22:33Z

Test build #65536 has finished for PR 15093 at commit 8a3d293.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tdas · 2016-09-17T21:16:29Z

@HyukjinKwon @srowen
This PR when merged into branch-2.0 somehow created an empty file QuantileSummaries.scala that is failing the lint test as the Apache license header does not exist -
Commit - a3bba37
Empty file - https://github.com/apache/spark/blob/a3bba372abce926351335d0a2936b70988f19b23/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/QuantileSummaries.scala

I am not sure how exactly backporting a patch led to an empty file, but this does not seem right. I am reverting this commit in branch 2.0. Please make a new PR to fix this in branch 2.0 correctly.

srowen · 2016-09-17T21:23:23Z

Oh weird! no idea why that happened. Yeah I'll take care of it from here.

tdas · 2016-09-17T21:26:48Z

I reverted already!

…th/size which is O(n) This PR fixes all the instances which was fixed in the previous PR. To make sure, I manually debugged and also checked the Scala source. `length` in [LinearSeqOptimized.scala#L49-L57](https://github.com/scala/scala/blob/2.11.x/src/library/scala/collection/LinearSeqOptimized.scala#L49-L57) is O(n). Also, `size` calls `length` via [SeqLike.scala#L106](https://github.com/scala/scala/blob/2.11.x/src/library/scala/collection/SeqLike.scala#L106). For debugging, I have created these as below: ```scala ArrayBuffer(1, 2, 3) Array(1, 2, 3) List(1, 2, 3) Seq(1, 2, 3) ``` and then called `size` and `length` for each to debug. I ran the bash as below on Mac ```bash find . -name *.scala -type f -exec grep -il "while (.*\\.length)" {} \; | grep "src/main" find . -name *.scala -type f -exec grep -il "while (.*\\.size)" {} \; | grep "src/main" ``` and then checked each. Author: hyukjinkwon <gurwls223@gmail.com> Closes #15093 from HyukjinKwon/SPARK-17480-followup. (cherry picked from commit 86c2d39) Signed-off-by: Sean Owen <sowen@cloudera.com>

srowen · 2016-09-17T21:29:20Z

Yep, saw that. I re-merged this, and yes during conflict resolution QuantileSummaries.scala comes up as a file added only in the master branch, but when I choose to not take the change in the IDE, I see it actually resulted in adding an empty file. I made sure that was not part of the commit and pushed again. Looks as intended now: 5fd354b

…th/size which is O(n) ## What changes were proposed in this pull request? This PR fixes all the instances which was fixed in the previous PR. To make sure, I manually debugged and also checked the Scala source. `length` in [LinearSeqOptimized.scala#L49-L57](https://github.com/scala/scala/blob/2.11.x/src/library/scala/collection/LinearSeqOptimized.scala#L49-L57) is O(n). Also, `size` calls `length` via [SeqLike.scala#L106](https://github.com/scala/scala/blob/2.11.x/src/library/scala/collection/SeqLike.scala#L106). For debugging, I have created these as below: ```scala ArrayBuffer(1, 2, 3) Array(1, 2, 3) List(1, 2, 3) Seq(1, 2, 3) ``` and then called `size` and `length` for each to debug. ## How was this patch tested? I ran the bash as below on Mac ```bash find . -name *.scala -type f -exec grep -il "while (.*\\.length)" {} \; | grep "src/main" find . -name *.scala -type f -exec grep -il "while (.*\\.size)" {} \; | grep "src/main" ``` and then checked each. Author: hyukjinkwon <gurwls223@gmail.com> Closes apache#15093 from HyukjinKwon/SPARK-17480-followup.

HyukjinKwon added 2 commits September 14, 2016 21:50

Fix more same instances

fbc3419

Last instance

ecd3dde

srowen reviewed Sep 14, 2016
View reviewed changes

Revert unrelated changes and use funtional transformation for a case

996e292

HyukjinKwon commented Sep 15, 2016

View reviewed changes

Address comment

cb536a4

srowen reviewed Sep 17, 2016

View reviewed changes

HyukjinKwon added 2 commits September 17, 2016 22:31

Fix comments

f5e8131

Correct the comments more

8a3d293

asfgit closed this in 86c2d39 Sep 17, 2016

HyukjinKwon deleted the SPARK-17480-followup branch September 18, 2016 14:05

HyukjinKwon restored the SPARK-17480-followup branch September 18, 2016 14:05

HyukjinKwon deleted the SPARK-17480-followup branch September 18, 2016 14:08

Conversation

HyukjinKwon commented Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon commented Sep 14, 2016

Uh oh!

srowen Sep 14, 2016

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Sep 14, 2016

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Sep 14, 2016

Uh oh!

SparkQA commented Sep 14, 2016

Uh oh!

SparkQA commented Sep 14, 2016

Uh oh!

HyukjinKwon Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 15, 2016

Uh oh!

srowen commented Sep 16, 2016

Uh oh!

srowen Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 17, 2016

Uh oh!

HyukjinKwon commented Sep 17, 2016

Uh oh!

SparkQA commented Sep 17, 2016

Uh oh!

srowen commented Sep 17, 2016

Uh oh!

SparkQA commented Sep 17, 2016

Uh oh!

tdas commented Sep 17, 2016

Uh oh!

srowen commented Sep 17, 2016

Uh oh!

tdas commented Sep 17, 2016

Uh oh!

srowen commented Sep 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HyukjinKwon commented Sep 14, 2016 •

edited

Loading

HyukjinKwon Sep 15, 2016 •

edited

Loading